Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/win make #4682

Merged
merged 1 commit into from
Feb 13, 2018
Merged

Fix/win make #4682

merged 1 commit into from
Feb 13, 2018
+14 −3

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Feb 11, 2018 •
edited
Loading

Requesting review from @Kubuxu I'm not very familiar with IPFS's make rules or make in general so this may need changes.
In my testing this ~~~resolved~~~ partially resolves #4510 when using MSYS2 tools.
I'm going to look into gx detection

While working on this I noticed a problem I couldn't work around.
/bin/Rules.mk utilizes a symlink for the binary dependencies, on Windows, by default, users don't have permission to create symlinks with mklink, MSYS2's ln.exe simply copies the source to the target (not as a link). This ends up not being a problem in practice but I feel the need to mention it. Here's the patch I initially tried without success.

diff --git a/bin/Rules.mk b/bin/Rules.mk
index 1a1dd7962..a2576c303 100644
--- a/bin/Rules.mk
+++ b/bin/Rules.mk
@@ -16,7 +16,11 @@ PATH := $(realpath $(d)):$(PATH)

 $(TGTS_$(d)):
        rm -f $@$(binpostfix)
+ifeq ($(OS),Windows_NT)
+       cmd /C mklink "$@$(binpostfix)" "$(notdir $^)$(binpostfix)"
+else
        ln -s $(notdir $^) $@
+endif

 bin/gx-v%:
        @echo "installing gx $(@:bin/gx-%=%)"

@djdv
Copy link
Contributor Author

djdv commented Feb 11, 2018

I'm going to look into gx detection

I thought the makefiles tried to detect automatically if gx was already installed and use it if it was, I didn't notice IPFS_GX_USE_GLOBAL had to be set to use the system version of gx.

With these commits I'm able to clone the repo, set environment variables, build, and install go-ipfs on Windows under MSYS2 using make build and make install without issues. Both with bundled gx and system gx (when IPFS_GX_USE_GLOBAL is set to 1).

@whyrusleeping
Copy link
Member

@victorbjelkholm What is the latest on windows CI through jenkins?

@whyrusleeping whyrusleeping added the topic/windows Windows specific label Feb 12, 2018
@djdv
Copy link
Contributor Author

djdv commented Feb 12, 2018

Cygwin's ln works differently from Msys's, it will create a binary file that works as a symlink inside the Cygwin shell but seems to be causing issues.

==> Download complete!
==> using 'unzip' to extract binary from archive
rm -f bin/gx-go.exe
ln -s gx-go-v1.6.0.exe bin/gx-go.exe
gx.exe install --global
ERROR: req-check hook failed: fork/exec C:\Users\J\go\src\github.com\ipfs\go-ipf
s\bin\gx-go.exe: Access is denied.
make: *** [mk/gx.mk:8: gx-deps] Error 1

Now this only happens when gx is invoked from make, this isn't happening when I invoke bin/gx.exe install --global directly from the shell. I don't know who's bug this is.

For now we can just avoid linking altogether on Windows and just copy the binary every time. It's not ideal but I can't think of a way around it.

@victorb
Copy link
Member

victorb commented Feb 12, 2018

@whyrusleeping

What is the latest on windows CI through jenkins?

PR in progress is here: #4430

Windows tests seems to work fine, but it's not calling make to run them.

@djdv djdv mentioned this pull request Feb 13, 2018
Merged
Kubuxu
Kubuxu reviewed Feb 13, 2018
View reviewed changes
bin/Rules.mk Outdated
@@ -1,6 +1,10 @@
include mk/header.mk

dist_root_$(d)=/ipfs/QmT3CLJKJzWPuN4NAN4LLy69UpKskMF3AuYhXstKdn8V43
binpostfix :=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have binpostfix in this makefile system. You can use $(?exe) to access it.

Kubuxu
Kubuxu reviewed Feb 13, 2018
View reviewed changes
bin/Rules.mk Outdated
rm -f $@
ln -s $(notdir $^) $@
rm -f $@$(binpostfix)
ifeq ($(OS),Windows_NT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this, use the variable exported here: https://github.com/ipfs/go-ipfs/blob/master/mk/util.mk#L4

Kubuxu
Kubuxu reviewed Feb 13, 2018
View reviewed changes
mk/gx.mk Outdated
ifeq ($(OS),Windows_NT)
binpostfix = .exe
endif
gx-path = gx/ipfs/$(shell gx$(binpostfix) deps find $(1))/$(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gx in this file should be avialable from PATH variable so path or the postfix should not be needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work without it?

Kubuxu
Kubuxu requested changes Feb 13, 2018
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the diff comments.

@djdv
Copy link
Contributor Author

djdv commented Feb 13, 2018

@Kubuxu
Should be better now.

@Kubuxu
Copy link
Member

Kubuxu commented Feb 13, 2018

@magik6k could you look why Jenkins failed here?

Kubuxu
Kubuxu approved these changes Feb 13, 2018
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makefiles seem good. I would just love to know why Jenkins failed.

@magik6k magik6k mentioned this pull request Feb 13, 2018
Closed
@magik6k
Copy link
Member

magik6k commented Feb 13, 2018

It's a random test fail, I've seen it a few times, reported in #4698 (also re-run jenkins, it's green now)

@Kubuxu
Copy link
Member

Kubuxu commented Feb 13, 2018

@djdv can you try running make build twice in a row. It shouldn't be unpacking the file twice.

From another side, I don't see a reason this couldn't get merged. It improves the situation, it just might not be perfect.

@djdv
Fix make deps on Windows
e613e9d 
License: MIT
Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>
@djdv
Copy link
Contributor Author

djdv commented Feb 13, 2018

@Kubuxu
On my system it downloads and extracts gx and gx-go on the first run but re-uses them on the second run without downloading or extracting anything. The only time it gets them again is when I remove them with git clean -fdx.

Are you encountering double unpacking?

Also I will force push to restart the test if they're working now. I needed to amend the commit anyway (wrong email address was attached originall). No code changes.

@Kubuxu
Copy link
Member

Kubuxu commented Feb 13, 2018

@djdv no, but this is something I suspected that could happen reading the Makefiles.

@whyrusleeping good to merge

@whyrusleeping whyrusleeping merged commit 2a5f344 into ipfs:master Feb 13, 2018
@whyrusleeping
Copy link
Member

Thank you @djdv! Its really great to have someone holding the windows flag :)

@Stebalien
Copy link
Member

I can't wait for the day when we can depend on the WSL and cross compile for windows on windows...

@djdv djdv mentioned this pull request Mar 13, 2018
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu approved these changes

Assignees
No one assigned
Labels
topic/windows Windows specific
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make: check_gx & dist_get issues on Windows
6 participants
@djdv @whyrusleeping @victorb @Kubuxu @magik6k @Stebalien